From 1ab19a5af6799408a3090c9afec6081506a05f1d Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Fri, 30 Dec 2016 10:50:31 +1300 Subject: [PATCH] review changes, round 2 Mostly focussing on the ergonomics of the API - removes with_* methods with closures, and replaces generics with trait objects. --- src/bin/check.rs | 50 +++++++++++++------------- src/cargo/ops/cargo_check.rs | 13 ------- src/cargo/ops/cargo_compile.rs | 29 ++++++++------- src/cargo/ops/cargo_install.rs | 6 +++- src/cargo/ops/cargo_package.rs | 3 +- src/cargo/ops/cargo_rustc/mod.rs | 61 +++++++++++++++----------------- src/cargo/ops/mod.rs | 2 -- 7 files changed, 76 insertions(+), 88 deletions(-) delete mode 100644 src/cargo/ops/cargo_check.rs diff --git a/src/bin/check.rs b/src/bin/check.rs index 61fd7a33a..d8c4a7027 100644 --- a/src/bin/check.rs +++ b/src/bin/check.rs @@ -1,8 +1,9 @@ use std::env; +use cargo::core::Workspace; use cargo::ops::{self, CompileOptions, MessageFormat}; -use cargo::ops::with_check_ws; use cargo::util::{CliResult, Config}; +use cargo::util::important_paths::find_root_manifest_for_wd; pub const USAGE: &'static str = " Check a local package and all of its dependencies for errors @@ -75,28 +76,29 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { options.flag_frozen, options.flag_locked)?; - with_check_ws(options.flag_manifest_path.clone(), config, |ws| { - let opts = CompileOptions { - config: config, - jobs: options.flag_jobs, - target: options.flag_target.as_ref().map(|t| &t[..]), - features: &options.flag_features, - all_features: options.flag_all_features, - no_default_features: options.flag_no_default_features, - spec: ops::Packages::Packages(&options.flag_package), - mode: ops::CompileMode::Check, - release: options.flag_release, - filter: ops::CompileFilter::new(options.flag_lib, - &options.flag_bin, - &options.flag_test, - &options.flag_example, - &options.flag_bench), - message_format: options.flag_message_format, - target_rustdoc_args: None, - target_rustc_args: None, - }; + let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?; + let ws = Workspace::new(&root, config)?; - ops::compile(ws, &opts)?; - Ok(None) - }) + let opts = CompileOptions { + config: config, + jobs: options.flag_jobs, + target: options.flag_target.as_ref().map(|t| &t[..]), + features: &options.flag_features, + all_features: options.flag_all_features, + no_default_features: options.flag_no_default_features, + spec: ops::Packages::Packages(&options.flag_package), + mode: ops::CompileMode::Check, + release: options.flag_release, + filter: ops::CompileFilter::new(options.flag_lib, + &options.flag_bin, + &options.flag_test, + &options.flag_example, + &options.flag_bench), + message_format: options.flag_message_format, + target_rustdoc_args: None, + target_rustc_args: None, + }; + + ops::compile(&ws, &opts)?; + Ok(None) } diff --git a/src/cargo/ops/cargo_check.rs b/src/cargo/ops/cargo_check.rs deleted file mode 100644 index 2e78a9c0f..000000000 --- a/src/cargo/ops/cargo_check.rs +++ /dev/null @@ -1,13 +0,0 @@ -use core::Workspace; -use util::important_paths::find_root_manifest_for_wd; -use util::{CliResult, Config}; - -pub fn with_check_ws(flag_manifest_path: Option, - config: &Config, f: F) - -> CliResult> - where F: FnOnce(&Workspace) -> CliResult> -{ - let root = find_root_manifest_for_wd(flag_manifest_path, config.cwd())?; - let ws = Workspace::new(&root, config)?; - f(&ws) -} diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 38e078259..9e8de638d 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -24,6 +24,7 @@ use std::collections::HashMap; use std::path::PathBuf; +use std::sync::Arc; use core::{Source, Package, Target}; use core::{Profile, TargetKind, Profiles, Workspace, PackageIdSpec}; @@ -63,10 +64,9 @@ pub struct CompileOptions<'a> { } impl<'a> CompileOptions<'a> { - pub fn with_default(config: &Config, mode: CompileMode, f: F) -> T - where F: FnOnce(CompileOptions) -> T + pub fn default(config: &'a Config, mode: CompileMode) -> CompileOptions<'a> { - let opts = CompileOptions { + CompileOptions { config: config, jobs: None, target: None, @@ -80,8 +80,7 @@ impl<'a> CompileOptions<'a> { message_format: MessageFormat::Human, target_rustdoc_args: None, target_rustc_args: None, - }; - f(opts) + } } } @@ -136,13 +135,13 @@ pub enum CompileFilter<'a> { pub fn compile<'a>(ws: &Workspace<'a>, options: &CompileOptions<'a>) -> CargoResult> { - compile_with_exec(ws, options, &mut DefaultExecutor) + compile_with_exec(ws, options, Arc::new(DefaultExecutor)) } -pub fn compile_with_exec<'a, E: Executor>(ws: &Workspace<'a>, - options: &CompileOptions<'a>, - exec: &mut E) - -> CargoResult> { +pub fn compile_with_exec<'a>(ws: &Workspace<'a>, + options: &CompileOptions<'a>, + exec: Arc) + -> CargoResult> { for member in ws.members() { for key in member.manifest().warnings().iter() { options.config.shell().warn(key)? @@ -151,11 +150,11 @@ pub fn compile_with_exec<'a, E: Executor>(ws: &Workspace<'a>, compile_ws(ws, None, options, exec) } -pub fn compile_ws<'a, E: Executor>(ws: &Workspace<'a>, - source: Option>, - options: &CompileOptions<'a>, - exec: &mut E) - -> CargoResult> { +pub fn compile_ws<'a>(ws: &Workspace<'a>, + source: Option>, + options: &CompileOptions<'a>, + exec: Arc) + -> CargoResult> { let CompileOptions { config, jobs, target, spec, features, all_features, no_default_features, release, mode, message_format, diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index ce7285e21..5f97888f9 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -6,6 +6,7 @@ use std::fs::{self, File}; use std::io::prelude::*; use std::io::SeekFrom; use std::path::{Path, PathBuf}; +use std::sync::Arc; use semver::Version; use tempdir::TempDir; @@ -106,7 +107,10 @@ pub fn install(root: Option<&str>, check_overwrites(&dst, pkg, &opts.filter, &list, force)?; } - let compile = ops::compile_ws(&ws, Some(source), opts, &mut DefaultExecutor).chain_error(|| { + let compile = ops::compile_ws(&ws, + Some(source), + opts, + Arc::new(DefaultExecutor)).chain_error(|| { if let Some(td) = td_opt.take() { // preserve the temporary directory, so the user can inspect it td.into_path(); diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 8436fc380..a93fc234a 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -2,6 +2,7 @@ use std::fs::{self, File}; use std::io::SeekFrom; use std::io::prelude::*; use std::path::{self, Path}; +use std::sync::Arc; use flate2::read::GzDecoder; use flate2::{GzBuilder, Compression}; @@ -298,7 +299,7 @@ fn run_verify(ws: &Workspace, tar: &File, opts: &PackageOpts) -> CargoResult<()> mode: ops::CompileMode::Build, target_rustdoc_args: None, target_rustc_args: None, - }, &mut DefaultExecutor)?; + }, Arc::new(DefaultExecutor))?; Ok(()) } diff --git a/src/cargo/ops/cargo_rustc/mod.rs b/src/cargo/ops/cargo_rustc/mod.rs index 158e72aa2..0862a296c 100644 --- a/src/cargo/ops/cargo_rustc/mod.rs +++ b/src/cargo/ops/cargo_rustc/mod.rs @@ -58,8 +58,8 @@ pub type PackagesToBuild<'a> = [(&'a Package, Vec<(&'a Target, &'a Profile)>)]; /// A glorified callback for executing calls to rustc. Rather than calling rustc /// directly, we'll use an Executor, giving clients an opportunity to intercept /// the build calls. -pub trait Executor: Clone + Send + 'static { - fn init(&mut self, _cx: &Context) {} +pub trait Executor: Send + Sync + 'static { + fn init(&self, _cx: &Context) {} /// If execution succeeds, the ContinueBuild value indicates whether Cargo /// should continue with the build process for this package. fn exec(&self, cmd: ProcessBuilder, _id: &PackageId) -> Result { @@ -67,21 +67,18 @@ pub trait Executor: Clone + Send + 'static { Ok(ContinueBuild::Continue) } - fn exec_json(&self, - cmd: ProcessBuilder, - _id: &PackageId, - mut handle_stdout: F1, - mut handle_srderr: F2) - -> Result - where F1: FnMut(&str) -> CargoResult<()>, - F2: FnMut(&str) -> CargoResult<()>, - { - cmd.exec_with_streaming(&mut handle_stdout, &mut handle_srderr)?; + fn exec_json(&self, + cmd: ProcessBuilder, + _id: &PackageId, + handle_stdout: &mut FnMut(&str) -> CargoResult<()>, + handle_srderr: &mut FnMut(&str) -> CargoResult<()>) + -> Result { + cmd.exec_with_streaming(handle_stdout, handle_srderr)?; Ok(ContinueBuild::Continue) } } -/// A DefaultExecutorcalls rustc without doing anything else. It is Cargo's +/// A DefaultExecutor calls rustc without doing anything else. It is Cargo's /// default behaviour. #[derive(Copy, Clone)] pub struct DefaultExecutor; @@ -96,15 +93,15 @@ pub enum ContinueBuild { // Returns a mapping of the root package plus its immediate dependencies to // where the compiled libraries are all located. -pub fn compile_targets<'a, 'cfg: 'a, E: Executor>(ws: &Workspace<'cfg>, - pkg_targets: &'a PackagesToBuild<'a>, - packages: &'a PackageSet<'cfg>, - resolve: &'a Resolve, - config: &'cfg Config, - build_config: BuildConfig, - profiles: &'a Profiles, - exec: &mut E) - -> CargoResult> { +pub fn compile_targets<'a, 'cfg: 'a>(ws: &Workspace<'cfg>, + pkg_targets: &'a PackagesToBuild<'a>, + packages: &'a PackageSet<'cfg>, + resolve: &'a Resolve, + config: &'cfg Config, + build_config: BuildConfig, + profiles: &'a Profiles, + exec: Arc) + -> CargoResult> { let units = pkg_targets.iter().flat_map(|&(pkg, ref targets)| { let default_kind = if build_config.requested_target.is_some() { Kind::Target @@ -137,7 +134,7 @@ pub fn compile_targets<'a, 'cfg: 'a, E: Executor>(ws: &Workspace<'cfg>, // part of this, that's all done next as part of the `execute` // function which will run everything in order with proper // parallelism. - compile(&mut cx, &mut queue, unit, exec)?; + compile(&mut cx, &mut queue, unit, exec.clone())?; } // Now that we've figured out everything that we're going to do, do it! @@ -207,10 +204,10 @@ pub fn compile_targets<'a, 'cfg: 'a, E: Executor>(ws: &Workspace<'cfg>, Ok(cx.compilation) } -fn compile<'a, 'cfg: 'a, E: Executor>(cx: &mut Context<'a, 'cfg>, - jobs: &mut JobQueue<'a>, - unit: &Unit<'a>, - exec: &mut E) -> CargoResult<()> { +fn compile<'a, 'cfg: 'a>(cx: &mut Context<'a, 'cfg>, + jobs: &mut JobQueue<'a>, + unit: &Unit<'a>, + exec: Arc) -> CargoResult<()> { if !cx.compiled.insert(*unit) { return Ok(()) } @@ -229,7 +226,7 @@ fn compile<'a, 'cfg: 'a, E: Executor>(cx: &mut Context<'a, 'cfg>, let work = if unit.profile.doc { rustdoc(cx, unit)? } else { - rustc(cx, unit, exec)? + rustc(cx, unit, exec.clone())? }; let link_work1 = link_targets(cx, unit)?; let link_work2 = link_targets(cx, unit)?; @@ -243,13 +240,13 @@ fn compile<'a, 'cfg: 'a, E: Executor>(cx: &mut Context<'a, 'cfg>, // Be sure to compile all dependencies of this target as well. for unit in cx.dep_targets(unit)?.iter() { - compile(cx, jobs, unit, exec)?; + compile(cx, jobs, unit, exec.clone())?; } Ok(()) } -fn rustc(cx: &mut Context, unit: &Unit, exec: &mut E) -> CargoResult { +fn rustc(cx: &mut Context, unit: &Unit, exec: Arc) -> CargoResult { let crate_types = unit.target.rustc_crate_types(); let mut rustc = prepare_rustc(cx, crate_types, unit)?; @@ -337,12 +334,12 @@ fn rustc(cx: &mut Context, unit: &Unit, exec: &mut E) -> CargoResul state.running(&rustc); let cont = if json_messages { exec.exec_json(rustc, &package_id, - |line| if !line.is_empty() { + &mut |line| if !line.is_empty() { Err(internal(&format!("compiler stdout is not empty: `{}`", line))) } else { Ok(()) }, - |line| { + &mut |line| { // stderr from rustc can have a mix of JSON and non-JSON output if line.starts_with("{") { // Handle JSON lines diff --git a/src/cargo/ops/mod.rs b/src/cargo/ops/mod.rs index 4f3751890..ecc15e81d 100644 --- a/src/cargo/ops/mod.rs +++ b/src/cargo/ops/mod.rs @@ -1,4 +1,3 @@ -pub use self::cargo_check::with_check_ws; pub use self::cargo_clean::{clean, CleanOptions}; pub use self::cargo_compile::{compile, compile_with_exec, compile_ws, CompileOptions}; pub use self::cargo_compile::{CompileFilter, CompileMode, MessageFormat, Packages}; @@ -25,7 +24,6 @@ pub use self::cargo_pkgid::pkgid; pub use self::resolve::{resolve_ws, resolve_ws_precisely, resolve_with_previous}; pub use self::cargo_output_metadata::{output_metadata, OutputMetadataOptions, ExportInfo}; -mod cargo_check; mod cargo_clean; mod cargo_compile; mod cargo_doc; -- 2.30.2